Add Nexus worker service for server-to-worker commands#708
Add Nexus worker service for server-to-worker commands#708rkannan82 wants to merge 4 commits intokannan/add-worker-instance-key-to-wft-completefrom
Conversation
rkannan82
force-pushed
the
kannan/activity-cancel
branch
3 times, most recently
from
4b2e0d1 to
54edb8e
Compare
rkannan82
force-pushed
the
kannan/activity-cancel
branch
2 times, most recently
from
afb40a2 to
b920471
Compare
rkannan82
changed the title
rkannan82
force-pushed
the
kannan/activity-cancel
branch
from
b920471 to
84c0bac
Compare
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM, though would want to see end-to-end working including in an SDK if possible before approving
rkannan82
force-pushed
the
kannan/activity-cancel
branch
from
84c0bac to
28ea1dd
Compare
rkannan82
changed the base branch from
master
to
kannan/add-worker-instance-key-to-wft-complete
rkannan82
force-pushed
the
kannan/activity-cancel
branch
from
7d3bfb7 to
e0d0049
Compare
rkannan82
force-pushed
the
kannan/add-worker-instance-key-to-wft-complete
branch
from
61952c9 to
98350a9
Compare
rkannan82
force-pushed
the
kannan/activity-cancel
branch
from
e0d0049 to
6c21af4
Compare
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
LGTM, don't merge this until you've generated the code and verified that this API works across SDK and server.
There will probably be a separate code generation project that will construct the YAML from protos to prevent the need to define services in separate places but this is a good start IMHO.
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
This is our first Nexus service, so lots of things undecided. I added comments, but would like to hear others' opinions.
rkannan82
force-pushed
the
kannan/activity-cancel
branch
from
6c21af4 to
37ffb33
Compare
rkannan82
force-pushed
the
kannan/activity-cancel
branch
from
37ffb33 to
6c21af4
Compare
rkannan82
changed the title
temporal/api/nexusservices/workerservice/v1/request_response.proto
Outdated
Show resolved
Hide resolved
| message WorkerCommandsRequest { | ||
| repeated WorkerCommand commands = 1; | ||
|
|
||
| message WorkerCommand { |
There was a problem hiding this comment.
I would pull this out to a top level message and put it in temporal.api.worker.v1 to make it easier to use and available in other contexts, traditionally we have only put requests and responses in request_response.proto files.
There was a problem hiding this comment.
But since this is a new nexus service, shouldn't this be decoupled from the existing gRPC temporal.api.worker.v1 protos and exist in the separate temporal/api/nexusservices? Maybe temporal.api.nexusservices.worker.v1?
Do we know what other contexts we might want to use this message, or is that hypothetical?
There was a problem hiding this comment.
I have made the changes that Roey suggested. Here is my interpretation.
- Top level request/response is about the RPC. In this case, it is nexus protocol. So we keep it under nexusservices dir.
- WorkerCommand is not strictly tied to this protocol. So we move it out of nexusservices dir.
-As to whether this is useful in other places: I am not sure. We could repurpose these command protos in the server side when we create the internal tasks. But it is not good to couple them.
temporal/api/nexusservices/workerservice/v1/request_response.proto
Outdated
Show resolved
Hide resolved
temporal/api/nexusservices/workerservice/v1/request_response.proto
Outdated
Show resolved
Hide resolved
temporal/api/nexusservices/workerservice/v1/request_response.proto
Outdated
Show resolved
Hide resolved
temporal/api/nexusservices/workerservice/v1/request_response.proto
Outdated
Show resolved
Hide resolved
Made-with: Cursor
rkannan82
force-pushed
the
kannan/activity-cancel
branch
from
32523fb to
cfff488
Compare
…vity-cancel Made-with: Cursor
- Rename WorkerCommandsRequest/Response to ExecuteCommandsRequest/Response to match the operation name (bergundy feedback) - Change operation name from executeCommands to ExecuteCommands (PascalCase) - Add doc: results list must be 1:1 with commands list (Sushisource feedback) Made-with: Cursor
Move WorkerCommand, CancelActivityCommand, WorkerCommandResult, and CancelActivityResult to temporal.api.worker.v1 as top-level messages. request_response.proto now only contains ExecuteCommandsRequest and ExecuteCommandsResponse, following the repo convention. (bergundy feedback) Made-with: Cursor
5 participants
Summary
Defines a Nexus service for server-to-worker communication, starting with activity cancellation support.
Design Decision
We chose a generic command API (
ExecuteCommandsRequestwithoneofcommand types) instead of a cancel-specific API. This allows a future optimization to batch multiple commands (cancel, pause, etc) in a single request and deliver to a worker in one RPC.Files
temporal/api/nexusservices/workerservice/v1/request_response.proto- request response definitionstemporal/api/nexusservices/workerservice/v1/service.yaml- Nexus service definitionRelated